Skip to content

Text control should not redraw indefinitely#3260

Draft
basilevs wants to merge 1 commit into
eclipse-platform:masterfrom
basilevs:issue_3920_inifinite_redraw
Draft

Text control should not redraw indefinitely#3260
basilevs wants to merge 1 commit into
eclipse-platform:masterfrom
basilevs:issue_3920_inifinite_redraw

Conversation

@basilevs
Copy link
Copy Markdown
Contributor

@basilevs basilevs commented Apr 22, 2026

Non-empty Text control with search style schedules redraws on every event loop iteration. This happens even if all custom SWT code is removed.
Empty Text control does the same if a backgound color is set.

Thread [main] (Suspended (breakpoint at line 2150 in Widget))	
	Text(Widget).setNeedsDisplay(long, long, boolean) line: 2150	
	Display.windowProc(long, long, long) line: 6476	
	OS.objc_msgSend(long, long) line: not available [native method]	
	SWTSearchField(NSControl).stringValue() line: 122	
	Text.drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long) line: 751	
	Text.drawInteriorWithFrame_inView(long, long, NSRect, long) line: 697	
	Display.windowProc(long, long, long, long) line: 6661	
	OS.objc_msgSendSuper(objc_super, long, NSRect) line: not available [native method]	
	Text(Widget).drawRect(long, long, NSRect) line: 813	
	Text.drawRect(long, long, NSRect) line: 759	
	Display.windowProc(long, long, long) line: 6246	
	OS.objc_msgSendSuper(objc_super, long, long, long, long, boolean) line: not available [native method]	
	Display.applicationNextEventMatchingMask(long, long, long, long, long, long) line: 5569	
	Display.applicationProc(long, long, long, long, long, long) line: 5970	
	OS.objc_msgSend(long, long, long, long, long, boolean) line: not available [native method]	
	NSApplication.nextEventMatchingMask(long, NSDate, NSString, boolean) line: 92	
	Display.readAndDispatch() line: 3992	
      .....

See eclipse-platform/eclipse.platform.ui#3920

Affected patforms:

  • MacOS SDK 14.0 aarch64 - high CPU use
  • MacOS SDK 14.4 aarch64 - high CPU use
  • MacOS SDK 24.4 aarch64 - Display.asyncExec() never executes
  • GTK4

This PR fixes the problem on MacOS by reverting a fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=266206, conflicting with overall AppKit paint damage scheduling approach.

AppKit resets needsDisplay flag when control is drawn. If the flag is set asynchronously and draw methods set it, infinite redraw loop happens. This happens even on uncustomized controls, as AppKit's native draw methods do set it (in complex controls with overlaying cells).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 22, 2026

Test Results

  182 files  +   30    182 suites  +30   29m 21s ⏱️ + 7m 42s
4 727 tests +   61  4 703 ✅ +   58   24 💤 + 4  0 ❌ ±0 
6 836 runs  +1 182  6 665 ✅ +1 140  171 💤 +43  0 ❌ ±0 

Results for commit 5ce03e5. ± Comparison against base commit 17667a8.

This pull request skips 1 test.
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Text ‑ test_backspaceAndDelete

♻️ This comment has been updated with latest results.

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented Apr 22, 2026

I'm surprised the test does not fail in CI. Is UI context really set up there?

Other Mac OS users, could you please run it in your environment to evaluate whether the original problem is specific to my setup?

OS: Mac OS X, v.26.4.1, aarch64 / cocoa
Java vendor: Homebrew
Java runtime version: 25.0.2
Java version: 25.0.2

In any case, this PR is ready to merge.

@basilevs basilevs force-pushed the issue_3920_inifinite_redraw branch from 4a59a5f to b12270f Compare April 22, 2026 18:56
vogella added a commit to vogella/eclipse.platform.swt that referenced this pull request Apr 22, 2026
On cocoa, Text.drawInteriorWithFrame_inView_searchfield queried the
search field text via NSControl.stringValue to decide whether to draw
the cancel icon. That call schedules a setNeedsDisplay: on the view
from inside the paint pass. Widget.setNeedsDisplay then enqueues the
view in display.needsDisplay, which is flushed after paint, marking
the view dirty again and causing readAndDispatch() to spin forever.

Read the text through NSCell.stringValue instead, which bypasses the
control-level side effect. Also add the JUnit regression test from
PR eclipse-platform#3260 reproducing the loop.

Fixes eclipse-platform/eclipse.platform.ui#3920

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vogella added a commit to vogella/eclipse.platform.swt that referenced this pull request Apr 22, 2026
…gger

Picks up the more reliable reproducer from PR eclipse-platform#3260
(shell.setLayout + requestLayout + shell.open) and adds temporary
instrumentation so CI logs reveal which native call in
drawInteriorWithFrame_inView_searchfield posts the re-entrant
setNeedsDisplay that drives the loop.

- Text.drawInteriorWithFrame_inView / _searchfield: print a stable
  step marker to stderr before and after every significant native
  call; update Text.DEBUG_PAINT_STEP so Widget.setNeedsDisplay can
  attribute the trigger.
- Widget.setNeedsDisplay / setNeedsDisplayInRect: when called
  re-entrantly during paint (isPainting contains view) and a paint
  step is armed, log the offending step, view id and widget class,
  plus a stack trace for the first 3 occurrences.

Temporary: to be removed once the trigger is identified.

Refs eclipse-platform/eclipse.platform.ui#3920

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@basilevs
Copy link
Copy Markdown
Contributor Author

I suspect that CI is using Java based on an incompatible MacOS SDK and that masks some test failures. There is a major difference in behavior resulting from different SDKs JVM is build against.

@basilevs basilevs force-pushed the issue_3920_inifinite_redraw branch from b12270f to 7982d1f Compare April 24, 2026 13:39
@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented Apr 24, 2026

With @vogella help, I've managed to reproduce the problem on Mac SDK 14.0 too. It is less severe there, as Display.asyncExec() is not broken, but high CPU usage is still observed. I expect CI to fail now.

@basilevs basilevs force-pushed the issue_3920_inifinite_redraw branch from 7982d1f to bd543d3 Compare April 24, 2026 15:26
@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented Apr 24, 2026

The test failures happens on GTK4 CI in addition to expected MacOS. @HeikoKlare could you please check if the test conceptually sound?

Display.readAndDispatch() should almost always return false once UI is drawn and settles. This is normally the case (4 events per second). When the bug is active (Text background color is set) the event rate grows to 122 events per second on fast workstation (or to 60 events per second on a slow one).

I'm not sure what's going on with GTK, if the overall idea is sound, I'll investigate.

@basilevs basilevs force-pushed the issue_3920_inifinite_redraw branch 7 times, most recently from 3a75b47 to 268aad3 Compare April 24, 2026 20:04
@basilevs
Copy link
Copy Markdown
Contributor Author

The test probably can be simplified by just ensuring that UI thread sleeps for 10 ms at least sometimes.

@Phillipus
Copy link
Copy Markdown
Contributor

Phillipus commented Apr 25, 2026

From eclipse-platform/eclipse.platform.ui#3920 (comment)

@Phillipus #3260 now fails on all SDK versions. It effectively measures CPU load in idle state. I would appreciate your review. Thanks for the help so far.

I refactored the tests into a standalone snippet.

Sometimes the plain Text control (SWT.NONE) passed the tests but other times fails on the 2nd assertion (intensity < 1e-4):

Detected 18/171610 UI event streaks/idle event loop iterations per second, intensity 0.00010. Expected: 2/100_000.

But the Cancel/Search Text control with background color failed both assertions (eventStreakCount < 50 and intensity < 1e-4):

Detected 68/161349 UI event streaks/idle event loop iterations per second, intensity 0.00042. Expected: 2/100_000.

macOS 15.7.5 with Temurin 21 Java.

@vogella
Copy link
Copy Markdown
Contributor

vogella commented Apr 25, 2026

@basilevs if my commit helps feel free to cherry pick it and push a patch to SWT

@basilevs
Copy link
Copy Markdown
Contributor Author

Sometimes the plain Text control (SWT.NONE) passed the tests but other times fails on the 2nd assertion (intensity < 1e-4):

Thanks! I saw this instability too, but I've been writing it of as me accidentally moving mouse around. Will keep an eye on it.

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented Apr 25, 2026

@basilevs if my commit helps feel free to cherry pick it and push a patch to SWT

Thanks, maybe later. I'm thinking about GTK4 now. It blinks cursor at 60 Hz refresh rate, breaking my idle assumptions. Disabling blinking for tests does not feel right - it moves the environment further from production.

@basilevs basilevs force-pushed the issue_3920_inifinite_redraw branch from 268aad3 to 43016ca Compare April 25, 2026 15:17
@tobiasmelcher
Copy link
Copy Markdown
Contributor

I think the high cpu load comes from call ((NSSearchField) view).stringValue() in method Text#drawInteriorWithFrame_inView_searchfield. Internally Widget#setNeedsDisplay is called which forces a redraw event. I would say that a method like stringValue() which wants to read the content of a UI widget should not trigger a redraw event.

@basilevs basilevs marked this pull request as draft April 25, 2026 18:59
@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented Apr 25, 2026

I think the high cpu load comes from call ((NSSearchField) view).stringValue() in method Text#drawInteriorWithFrame_inView_searchfield. Internally Widget#setNeedsDisplay is called which forces a redraw event. I would say that a method like stringValue() which wants to read the content of a UI widget should not trigger a redraw event.

Unfortunately, this method delegates to MacOS and there is not much that we can do about that:

public NSString stringValue() {
	long result = OS.objc_msgSend(this.id, OS.sel_stringValue);
	return result != 0 ? new NSString(result) : null;
}

Lars Vogel has suggested a fix where the method is not called in the hotspot. That visibly solves the problem.

@basilevs basilevs force-pushed the issue_3920_inifinite_redraw branch from 43016ca to 55305c7 Compare April 25, 2026 21:24
@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 1, 2026

@squarti please review

@merks
Copy link
Copy Markdown
Contributor

merks commented May 1, 2026

I'm not sure what you mean by a feature patch, but it's perfectly acceptable to update the Platform's contribution at any time up until the expect contribution for M3 which should then remain stable. In other words, updates before May 15 are fine and then should remain stable from May 15 through May 21 (when I promote staging).

@Phillipus
Copy link
Copy Markdown
Contributor

The changes here are as you say "dramatic" and go deep into the Mac code. From past experience I would be wary of merging this in 4.40. This could have side effects elsewhere that manifest further down the line.

Right now the main impact of this issue manifests on Mac JDKs that have been compiled with Mac SDK 26. So far only the Homebrew JDK is compiled with that. And because the Equinox launcher is compiled with SDK 15 the issue does not manifest in Eclipse itself.

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 1, 2026

Right now the main impact of this issue manifests on Mac JDKs that have been compiled with Mac SDK 26. So far only the Homebrew JDK is compiled with that. And because the Equinox launcher is compiled with SDK 15 the issue does not manifest in Eclipse itself.

Not true. Elevated CPU use is reproducible on all JDKs and all SDKs.

@Phillipus
Copy link
Copy Markdown
Contributor

Phillipus commented May 1, 2026

Not true. Elevated CPU use is reproducible on all JDKs and all SDKs.

I was referring to the problem on dark theme and Text controls not updating. Right now "elevated CPU use" is not a deal breaker. Your proposed changes are, as you say, "dramatic" and are major changes to the Display and Widget classes, not just Text, and could be a deal breaker if there is not enough regression testing.

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 1, 2026

I'm not sure what you mean by a feature patch, but it's perfectly acceptable to update the Platform's contribution at any time up until the expect contribution for M3 which should then remain stable. In other words, updates before May 15 are fine and then should remain stable from May 15 through May 21 (when I promote staging).

The changes are major, so more manual testing is required before merge. My tests while running PDE from sources are not enough. I have to prepare a patch and install it into daily-driver PDE to look for regressions during daily use.

@Phillipus
Copy link
Copy Markdown
Contributor

Phillipus commented May 1, 2026

I did a quick smoke test with this PR with our RCP app and found a major problem. When a text control in a GEF diagram gets the focus the whole background goes blank. That should not happen.

Screen.Recording.2026-05-01.at.16.00.17.mov

These changes have impact.

@Phillipus
Copy link
Copy Markdown
Contributor

Phillipus commented May 1, 2026

And other areas of the UI are not being painted. In the following screen cast the tabbed property items are blank when a new object is selected. They are redrawn when the mouse passes over them:

Screen.Recording.2026-05-01.at.16.12.52.mov

@vogella
Copy link
Copy Markdown
Contributor

vogella commented May 1, 2026

@basilevs running an aggregator build for the sdk is trivial these days at least under Linux (hence I assume Mac too). Takes only 4 minutes on my machine. Process: clone aggregator, update submodules, get any change you want into the submodule and run the Maven build (skipping tests) with -T6

@Phillipus
Copy link
Copy Markdown
Contributor

Phillipus commented May 1, 2026

And I'm seeing editor parts tabs not painted until the mouse hovers over them:

Screen.Recording.2026-05-01.at.16.19.49.mov

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 1, 2026

@Phillipus which GEF are you using? Is it FX variant?

@Phillipus
Copy link
Copy Markdown
Contributor

Phillipus commented May 1, 2026

@Phillipus which GEF are you using? Is it FX variant?

I use the latest GEF Classic I-build. This has nothing to do with it. Look at this comment and this comment - those are SWT components not GEF.

As I said before the changes in this PR are affecting how the UI is drawn in various places not necessarily related to GEF. And those are the ones I found initially with a quick test, there could be more.

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 1, 2026

Could not create a feature-patch - getting version conflicts. Every time I need one there are always issues.

Reproduced the CTabFolder blank tab area in debugger, thanks Phillipus.

This effectively reverts 9f118a8 - a
fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=266206

AppKit resets needsDisplay flag when control is drawn. If the flag is
set asynchronously and draw methods set it, infinite redraw loop
happens. This happens even on uncustomized controls, as AppKit's native
draw methods do set it (in complex controls with overlaying cells).
@basilevs basilevs force-pushed the issue_3920_inifinite_redraw branch from 9679497 to 5ce03e5 Compare May 1, 2026 17:52
* Since macOS 14 the clipsToBounds property of NSView has to be set to true
* See https://developer.apple.com/documentation/macos-release-notes/appkit-release-notes-for-macos-14
*/
OS.objc_msgSend(id, OS.sel_setClipsToBounds_, true);
Copy link
Copy Markdown
Contributor Author

@basilevs basilevs May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed CTabFolder update by restoring setClipsToBounds. I will keep trying to setup field testing and create a test covering CTabFolder regression scenario.

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 2, 2026

@basilevs running an aggregator build for the sdk is trivial these days at least under Linux (hence I assume Mac too). Takes only 4 minutes on my machine. Process: clone aggregator, update submodules, get any change you want into the submodule and run the Maven build (skipping tests) with -T6

AFAIK, aggregator needs changes to be already published (works with update sites). Mine are not merged yet.
Also, prexisitng features depend on pre-published plugins, so I see no way to "get a change into the submodule".

@vogella
Copy link
Copy Markdown
Contributor

vogella commented May 2, 2026

No, you can pull in changes from other repos before the build. I do it all the time

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 2, 2026

No, you can pull in changes from other repos before the build. I do it all the time

I do not have a repo. Even if I had, I would have to somehow override or recreate existing features.

@vogella
Copy link
Copy Markdown
Contributor

vogella commented May 3, 2026

No, you can pull in changes from other repos before the build. I do it all the time

I do not have a repo. Even if I had, I would have to somehow override or recreate existing features.

You have a clone, here is an example how to do that.

 #!/usr/bin/env bash
  set -euo pipefail

  REPO=${1:?repo dir, e.g. eclipse.platform.ui}
  PR=${2:?PR number}

  # 1. update aggregator + submodules
  git pull --rebase
  git submodule update --init --recursive

  # 2. pull the PR into the submodule
  git -C "$REPO" fetch origin "pull/$PR/head:pr-$PR"
  git -C "$REPO" checkout "pr-$PR"

  # 3. build the SDK
  mvn clean verify -Pbuild-individual-bundles -DskipTests

  Usage: ./apply_pr.sh eclipse.platform.ui 2345

I will stop trying to convince you, if you prefer a different way of testing, that is fine for me.

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 3, 2026

@vogella I think you have a different aggregator in mind, which confused me. I was talking about https://github.com/eclipse-simrel/simrel.build , for which the flow you describe do not seem to apply.

I will re-read contribution guide in an attempt to realign.

@merks
Copy link
Copy Markdown
Contributor

merks commented May 3, 2026

I was wondering which wires were crossed because Lars’ advice was sound. You can build the simrel aggregation from the context menu of the .aggr in the Oomphed IDE for simrel. I would show a picture but I’m on my phone.

@vogella
Copy link
Copy Markdown
Contributor

vogella commented May 3, 2026

@vogella I think you have a different aggregator in mind, which confused me. I was talking about https://github.com/eclipse-simrel/simrel.build , for which the flow you describe do not seem to apply.

I will re-read contribution guide in an attempt to realign.

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator

@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 3, 2026

I've built the aggregator by instructions from Lars and the resulting SDK tests fine. I had to install Oomph Targlets separately to make it useful. I'll try it out for a while.

Thanks for all the advice.

basilevs added a commit to basilevs/eclipse.platform.swt that referenced this pull request May 10, 2026
A regression test for eclipse-platform#3260 where tabs fail to render if
`NSView.setClipsToBounds()` is omited before scheduling redraw.

This test fails to reproduce the problem, but is useful as rendering is
not tested at all in this suite.
basilevs added a commit to basilevs/eclipse.platform.swt that referenced this pull request May 10, 2026
A regression test for eclipse-platform#3260 where tabs fail to render if
`NSView.setClipsToBounds()` is omited before scheduling redraw.

This test fails to reproduce the problem, but is useful as rendering is
not tested at all in this suite.
@basilevs
Copy link
Copy Markdown
Contributor Author

basilevs commented May 10, 2026

I've tested CTabItem rendering in an attempt to prevent the regression found by @Phillipus and got very interesting results on Linux and MacOS GHA. The rendering is broken on some environments, but not on others.

basilevs added a commit to basilevs/eclipse.platform.swt that referenced this pull request May 10, 2026
A regression test for eclipse-platform#3260 where tabs fail to render if
`NSView.setClipsToBounds()` is omited before scheduling redraw.

This test fails to reproduce the problem, but is useful as rendering is
not tested at all in this suite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants